Implement a token bucket used for adaptive retries#1
Implement a token bucket used for adaptive retries#1ubaskota merged 5 commits intoadaptive_retriesfrom
Conversation
nateprewitt
left a comment
nateprewitt
left a comment
There was a problem hiding this comment.
Good start! I have some questions and feedback. Let me know if you need anything clarified or if I'm missing something.
| async with token_bucket._lock: # type: ignore | ||
| token_bucket._refill() # type: ignore |
There was a problem hiding this comment.
I think we talked about this previously, but we don't want to be calling private methods if we can avoid it. Is there another way we can test this that better reflects the public interfaces?
There was a problem hiding this comment.
Yeah, we did talk about this earlier. I updated the other tests to use a public method, but for this we are specifically testing whether the refill exceeds the max capacity when the rate is increased, so I decided to leave it as it is.
jonathan343
left a comment
jonathan343
left a comment
There was a problem hiding this comment.
very minor nit. Looks good to me otherwise
| """ | ||
| TokenBucket provides a collection of arbitrary tokens while managing issuance | ||
| and refilling over time. This is controlled by a fill rate that can be variably | ||
| adjusted. When tokens aren't available, the bucket will enforce a delay before | ||
| attempting to reacquire tokens until one is available or the defined timeout is | ||
| reached. | ||
| """ |
There was a problem hiding this comment.
nit - See the multi-line docstring guidance from PEP 257 below:
Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description. The summary line may be used by automatic indexing tools; it is important that it fits on one line and is separated from the rest of the docstring by a blank line. The summary line may be on the same line as the opening quotes or on the next line. The entire docstring is indented the same as the quotes at its first line (see example below).
I've provided my suggestion below:
| """ | |
| TokenBucket provides a collection of arbitrary tokens while managing issuance | |
| and refilling over time. This is controlled by a fill rate that can be variably | |
| adjusted. When tokens aren't available, the bucket will enforce a delay before | |
| attempting to reacquire tokens until one is available or the defined timeout is | |
| reached. | |
| """ | |
| """Token bucket for rate limiting with configurable fill rate. | |
| Manages token issuance and automatic refilling over time. When tokens aren't | |
| available, enforces a delay until tokens are refilled or timeout is reached. | |
| """ |
3 participants
Issue #, if available:
Description of changes:
Implements an async token bucket that will be used for client side rate limiting in Adaptive retries. The
TokenBucketclass provides configurable rate limiting with automatic token refill over time.Testing:
Known Limitation:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.